Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

quic: another round of refactorings #34247

Closed
wants to merge 16 commits into from
Closed

quic: another round of refactorings #34247

wants to merge 16 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jul 7, 2020 •
edited
Loading

This is a WIP, more will be added:

Commits:

  • quic: additional minor cleanups in node_quic_session.h

    Minor miscellaneous cleanups

  • quic: remove unnecessary bool conversion

    Self-explanatory

  • quic: handle errors thrown / rejections in the session event

  • quic: refactor/improve error handling for busy event

  • quic: add tests confirming error handling for QuicSocket close event

  • quic: refactor/improve QuicSocket ready event handling

  • quic: refactor/improve QuicSocket ready event handling

    Starting to improve/refactor/verify error handling on events

  • quic: use Number() instead of bigint for QuicSocket stats

  • quic: unref timers again

    Fix a bug

  • quic: use getter/setting for stateless reset toggle

    Improve API ergonomics

  • quic: cleanup QuicSocketFlags, used shared state struct

    Use AliasedStruct for QuicSocket shared state, cleanup no longer used state flags

  • quic: proper custom inspect for QuicEndpoint/QuicSocket/QuicSession/QuicStream

    Self explanatory

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. quic Issues and PRs related to the QUIC implementation / HTTP/3. labels Jul 7, 2020
@jasnell jasnell added the experimental Issues and PRs related to experimental features. label Jul 7, 2020
@jasnell
quic: handle errors thrown / rejections in the session event
ebc8f5e 
Errors thrown within the session event handler will be handled
by destroying the session (allowing a proper connection close
to be sent to the client peer). They will not crash the parent
QuicSocket by default. Instead, a `'sessionError'` event will
be emitted, allowing the error to be logged or handled.
@jasnell

This comment has been minimized.

Sign in to view
@addaleax

This comment has been minimized.

Sign in to view
@jasnell

This comment has been minimized.

Sign in to view
@addaleax

This comment has been minimized.

Sign in to view
@addaleax

This comment has been minimized.

Sign in to view
@jasnell

This comment has been minimized.

Sign in to view
@addaleax

This comment has been minimized.

Sign in to view
@addaleax @jasnell
quic: unref timers again
71d0a18 
0f97d60 accidentally removed this.

Refs: nodejs#34186
@jasnell

This comment has been minimized.

Sign in to view
@jasnell jasnell force-pushed the quic-cleanups-4 branch 2 times, most recently from c767d37 to f044069 Compare July 8, 2020 16:06
@jasnell
quic: cleanup QuicSocketFlags, used shared state struct
17089ea 
Some of the flags were no longer being used.

Switched to use an AliasedStruct for shared state to avoid
extraneous expensive JS=>C++ calls.

Removed unused QuicSocket option
@jasnell jasnell force-pushed the quic-cleanups-4 branch 2 times, most recently from e5cea72 to 2a61277 Compare July 8, 2020 17:05
@jasnell jasnell marked this pull request as ready for review July 8, 2020 17:07
@jasnell jasnell requested a review from a team July 8, 2020 17:07
@jasnell
Copy link
Member Author

jasnell commented Jul 8, 2020

Ping @nodejs/quic... this is ready for review

@jasnell
Copy link
Member Author

jasnell commented Jul 8, 2020

Both CI's are good.

@github-actions github-actions bot mentioned this pull request Jul 9, 2020
Open
20 tasks
jasnell added a commit that referenced this pull request Jul 9, 2020
@jasnell
quic: additional minor cleanups in node_quic_session.h
c535131 
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
@jasnell
quic: remove unnecessary bool conversion
bcde849 
The argument will always be a boolean already

PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
@jasnell
quic: handle errors thrown / rejections in the session event
edc71ef 
Errors thrown within the session event handler will be handled
by destroying the session (allowing a proper connection close
to be sent to the client peer). They will not crash the parent
QuicSocket by default. Instead, a `'sessionError'` event will
be emitted, allowing the error to be logged or handled.

PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
@jasnell
quic: refactor/improve error handling for busy event
cc89aac 
Also, change setServerBusy into a setter

PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
@jasnell
quic: add tests confirming error handling for QuicSocket close event
e381326 
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
@jasnell
quic: refactor/improve QuicSocket ready event handling
afc9390 
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
@jasnell
quic: refactor/improve/document QuicSocket listening event
94372b1 
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
@jasnell
quic: use Number() instead of bigint for QuicSocket stats
7123609 
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell pushed a commit that referenced this pull request Jul 9, 2020
@addaleax @jasnell
quic: unref timers again
f2753c7 
0f97d60 accidentally removed this.

Refs: #34186

PR-URL: #34247
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell added a commit that referenced this pull request Jul 9, 2020
@jasnell
quic: use getter/setter for stateless reset toggle
d08e99d 
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
@jasnell
quic: cleanup QuicSocketFlags, used shared state struct
fe11f6b 
Some of the flags were no longer being used.

Switched to use an AliasedStruct for shared state to avoid
extraneous expensive JS=>C++ calls.

Removed unused QuicSocket option

PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
@jasnell
quic: proper custom inspect for QuicEndpoint
511f8c1 
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
@jasnell
quic: proper custom inspect for QuicSocket
b047930 
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
@jasnell
quic: proper custom inspect for QuicSession
0860b11 
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
@jasnell
quic: proper custom inspect for QuicStream
458d243 
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jul 9, 2020
@jasnell
quic: remove no longer valid CHECK
26493c0 
PR-URL: #34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
Copy link
Member Author

jasnell commented Jul 9, 2020

Landed in e8f5745...26493c0

@jasnell jasnell closed this Jul 9, 2020
This was referenced Jul 10, 2020
Open
Open
Open
Open
Open
@MylesBorins
Copy link
Contributor

Marked as "backport-requested" because I do not believe quic is on 14.x yet. Should we change the label to "dont-land"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. experimental Issues and PRs related to experimental features. quic Issues and PRs related to the QUIC implementation / HTTP/3.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jasnell @addaleax @nodejs-github-bot @MylesBorins